Skip to content

Automatic config generation for automatic delay cal selection#113

Open
tikk3r wants to merge 18 commits into
masterfrom
auto-auto-delay
Open

Automatic config generation for automatic delay cal selection#113
tikk3r wants to merge 18 commits into
masterfrom
auto-auto-delay

Conversation

@tikk3r
Copy link
Copy Markdown
Member

@tikk3r tikk3r commented Apr 1, 2026

No description provided.

@tikk3r
Copy link
Copy Markdown
Member Author

tikk3r commented Apr 4, 2026

Core functionality all seems to work now. Just stuck on why the generate_skymodels step is still executed even though the array length checked in the when condition should be >0.

@tikk3r
Copy link
Copy Markdown
Member Author

tikk3r commented Apr 16, 2026

why the generate_skymodels step is still executed even though the array length checked in the when condition should be >0.

This looks like it's maybe just a Toil oddity.

Functionality all works, but the vlass starting models are not used it seems. A manual facetselfcal run against the same VLASS image yields wildly different astrometry even if I completely remove the generat_skymodels step from the CWL code as a test, so I suspect the pipeline is still generating a point source and passing that.

Left to right: VLASS image, pipeline image 9, manual image 3

image

@tikk3r
Copy link
Copy Markdown
Member Author

tikk3r commented Apr 17, 2026

It looks like the difference may be in the PyBDSF commands for source finding. The facetselfcal-generated model has more components than what skynet outputs:

Skynet

# (Name, Type, Patch, Ra, Dec, I, Q, U, V, MajorAxis, MinorAxis, Orientation, ReferenceFrequency='144e+06', SpectralIndex='[]', LogarithmicSI) = format
, , P0, 00:00:00, +00.00.00
ME0,GAUSSIAN,P0,10:42:07.07427757,+56.20.35.11342256,0.7501937174536204,0.0,0.0,0.0,1.6648132122494166,1.2344896254335085,109.43104961290301,144e+06,[-0.7],true
ME1,GAUSSIAN,P0,10:42:06.08426152,+56.20.42.68432263,0.0933097437405896,0.0,0.0,0.0,1.6331785957898932,0.34046779089727963,136.72680994037083,144e+06,[-0.7],true

Facetselfcal

format = Name, Type, Patch, Ra, Dec, I, Q, U, V, MajorAxis, MinorAxis, Orientation, ReferenceFrequency='3.00000e+09', SpectralIndex='[]'

, , ILTJ104206.59+562039.5_vlass_patch_s0, 00:00:00, +00.00.00
ILTJ104206.59+562039.5_vlass_w0_i0_s0_g0, POINT, ILTJ104206.59+562039.5_vlass_patch_s0, 10:42:7.119336, +56.20.35.033496, 2.555e-02, 0.0, 0.0, 0.0, 0.00000e+00, 0.00000e+00, 0.00000e+00, 3.00000e+09, [-0.8]
ILTJ104206.59+562039.5_vlass_w0_i0_s0_g2, POINT, ILTJ104206.59+562039.5_vlass_patch_s0, 10:42:6.991234, +56.20.35.438892, 1.120e-02, 0.0, 0.0, 0.0, 0.00000e+00, 0.00000e+00, 0.00000e+00, 3.00000e+09, [-0.8]
ILTJ104206.59+562039.5_vlass_w0_i0_s0_g3, GAUSSIAN, ILTJ104206.59+562039.5_vlass_patch_s0, 10:42:6.768397, +56.20.35.979193, 6.761e-03, 0.0, 0.0, 0.0, 4.41003e+00, 0.00000e+00, 1.68543e+02, 3.00000e+09, [-0.8]
, , ILTJ104206.59+562039.5_vlass_patch_s1, 00:00:00, +00.00.00
ILTJ104206.59+562039.5_vlass_w0_i0_s1_g1, POINT, ILTJ104206.59+562039.5_vlass_patch_s1, 10:42:6.086021, +56.20.42.687386, 3.819e-03, 0.0, 0.0, 0.0, 0.00000e+00, 0.00000e+00, 0.00000e+00, 3.00000e+09, [-0.8]
, , ILTJ104206.59+562039.5_vlass_patch_s2, 00:00:00, +00.00.00
ILTJ104206.59+562039.5_vlass_w0_i1_s2_g4, GAUSSIAN, ILTJ104206.59+562039.5_vlass_patch_s2, 10:42:4.669273, +56.21.1.664814, 2.484e-03, 0.0, 0.0, 0.0, 2.34113e+00, 9.84720e-01, 6.75894e+01, 3.00000e+09, [-0.8]

@tikk3r
Copy link
Copy Markdown
Member Author

tikk3r commented Apr 21, 2026

After some digging around in skynet.py it is indeed related to how it manipulates the sky model found by PyBDSF. Writing out the PyBDSF model directly results in the same model as what facetselfcal generates.

This means as far as I can tell the goal of this branch is there, so I'll mark it ready for review and we can fix skynet in another one.

@tikk3r tikk3r marked this pull request as ready for review April 21, 2026 20:09
Copy link
Copy Markdown
Member

@lonbar lonbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I found a few nits.

The CI fails because of the issue solved in #116, so will be fixed when the branch is rebased.

Comment thread workflows/subworkflows/find-best-delay-calibrator.cwl
Comment thread workflows/delay-calibration.cwl
Comment thread workflows/delay-calibration.cwl Outdated
Comment thread steps/lofar_vlbi_plot.cwl
Comment thread workflows/subworkflows/find-best-delay-calibrator.cwl Outdated
@lonbar
Copy link
Copy Markdown
Member

lonbar commented Apr 23, 2026

This looks like it's maybe just a Toil oddity.

@tikk3r could you create an issue for this on our issue tracker? I can look into it later.

@tikk3r
Copy link
Copy Markdown
Member Author

tikk3r commented May 7, 2026

Alright, I finally figured out what is going wrong in this branch. I still think there are issues with how skynet manipulates the model, but that is not the main cause of the selfcal problem here. For some reason cwltool (or at least my version 3.1.20250110105449) validates the workflow despite there being a logic error in it. The model is generated via this call:

    - id: generate_skymodels
      in:
        - id: msin
          source: msin
          valueFrom: $(self[0])
        - id: delay_calibrator
          source: delay_calibrator
        - id: process_all
          default: true
        - id: starting_skymodel
          default: starting_skymodel
      out: 
        - id: skymodel
        - id: logfile
      run: ../../steps/delay_cal_model.cwl
      when: $(inputs.starting_skymodel == null)

however, delay_cal_model.cwl does not have a starting_skymodel input ; only model_image. It doesn't catch this apparently.

@tikk3r tikk3r linked an issue May 12, 2026 that may be closed by this pull request
@tikk3r
Copy link
Copy Markdown
Member Author

tikk3r commented May 13, 2026

The issue with wrong skymodels seems to be resolved after remove the unnecessary step from the subworkflow. Here's an updated comparison between the selfcal and the VLASS model, now showing good agreement in astrometry as would be expected.

image

@tikk3r tikk3r force-pushed the auto-auto-delay branch from cbd97fa to 24fbd03 Compare May 13, 2026 10:08
@tikk3r
Copy link
Copy Markdown
Member Author

tikk3r commented May 13, 2026

This looks like it's maybe just a Toil oddity.

@tikk3r could you create an issue for this on our issue tracker? I can look into it later.

Do we still want this now that you've found a Toil ticket and opened a PR about it?

@tikk3r tikk3r requested a review from lonbar May 13, 2026 10:11
lonbar
lonbar previously approved these changes May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tie automatic DI settings into automatic delay calibrator selection

2 participants